-
Notifications
You must be signed in to change notification settings - Fork 131
feat(rpc): add OpenTelemetry tracing #863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
swift1337
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big! Left comments
| func (b *Backend) GetCode(address common.Address, blockNrOrHash rpctypes.BlockNumberOrHash) (hexutil.Bytes, error) { | ||
| blockNum, err := b.BlockNumberFromComet(blockNrOrHash) | ||
| func (b *Backend) GetCode(ctx context.Context, address common.Address, blockNrOrHash rpctypes.BlockNumberOrHash) (bz hexutil.Bytes, err error) { | ||
| ctx, span := tracer.Start(ctx, "GetCode", trace.WithAttributes(attribute.String("address", address.String()), attribute.String("blockNorHash", unwrapBlockNOrHash(blockNrOrHash)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a lot of repeated code that can be simplified with a small wrapper. Consider the following:
Creating span for existing ctx:
// wrapper would be located here:
// import https://github.com/cosmos/evm/trace
// func Start(ctx context.Context, name string, attr... Attribute)
// trace.Str supports string or Stringable
// trace.Block automatically parses block or hash
// ... other cosmos/evm specific attrs
ctx, span := trace.Start(ctx, "GetCode", trace.Str("address", address), trace.Block("block", blockNrOrHash))
// regular span end
defer span.End()
// captures err if not nil
defer span.EndErr(err)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating span for new ctx:
ctx, span := trace.StartNew("GetCode", attrs...)
defer span.End()There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like the idea of span.EndErr, doing all the cleanup in one go, i could add this. for the other bits of the wrapper, im indifferent. having to do context.Background for a new span is pretty rare (geth API design weirdness). limiting the wrapper to only attributes could be a footgun later down the line if more StartSpanOptions are added that we may want to utilize. currently there are 2 (WithAttributes, WithTimestamp). if you feel pretty strongly though, i'm open to discussing more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like the End() and EndErr()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be good to make a trace wrapper in the sdk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well span.End is already a thing, and adding EndErr would require me to create a wrapper type around the span.
i have already made a change to make ending a span with errors cleaner: add span EndSpanErr helper function
| "github.com/cosmos/cosmos-sdk/server" | ||
| ) | ||
|
|
||
| var tracer = otel.Tracer("evm/rpc/namespaces/ethereum/debug") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same approach:
// import github.com/cosmos/evm/trace
var tracer = trace.New("evm/rpc/namespaces/ethereum/debug")|
Curious, where do we configure tracing sampling rate here? |
can be configured on the otel collector side: https://opentelemetry.io/docs/concepts/sampling/ and can be configured client side via otelconf https://github.com/open-telemetry/opentelemetry-configuration/blob/main/examples/kitchen-sink.yaml#L280 we have this instantiated automatically in cosmos sdk here: https://github.com/cosmos/cosmos-sdk/blob/cfaffe2b286442a262e506845cdf2d05fb6ed2dc/telemetry/config.go |
|
@technicallyty conflicts |
| resBlock *cmtrpctypes.ResultBlock, | ||
| blockRes *cmtrpctypes.ResultBlockResults, | ||
| ) []*evmtypes.MsgEthereumTx { | ||
| ctx, span := tracer.Start(ctx, "EthMsgsFromCometBlock") |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix the problem, replace the assignment to ctx on line 110 with the blank identifier _. This makes it clear to future maintainers that the new context object returned by tracer.Start is intentionally not used, and prevents misleading anyone into thinking that ctx is meant to change in this function. Only the span variable is needed for proper tracing. No further imports, method definitions, or code changes are necessary outside this line.
-
Copy modified line R110
| @@ -107,7 +107,7 @@ | ||
| resBlock *cmtrpctypes.ResultBlock, | ||
| blockRes *cmtrpctypes.ResultBlockResults, | ||
| ) []*evmtypes.MsgEthereumTx { | ||
| ctx, span := tracer.Start(ctx, "EthMsgsFromCometBlock") | ||
| _, span := tracer.Start(ctx, "EthMsgsFromCometBlock") | ||
| defer span.End() | ||
| var result []*evmtypes.MsgEthereumTx | ||
| block := resBlock.Block |
| // BlockBloom query block bloom filter from block results | ||
| func (b *Backend) BlockBloomFromCometBlock(blockRes *cmtrpctypes.ResultBlockResults) (ethtypes.Bloom, error) { | ||
| func (b *Backend) BlockBloomFromCometBlock(ctx context.Context, blockRes *cmtrpctypes.ResultBlockResults) (result ethtypes.Bloom, err error) { | ||
| ctx, span := tracer.Start(ctx, "BlockBloomFromCometBlock") |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
The best way to fix this problem is to avoid assigning the new context at all; instead, assign only the needed span variable and discard the new ctx with the blank identifier _.
Specifically: replace
ctx, span := tracer.Start(ctx, "BlockBloomFromCometBlock")with
_, span := tracer.Start(ctx, "BlockBloomFromCometBlock")This makes clear to readers (and to linters) that the context is intentionally ignored and that the assignment to ctx was not necessary.
Only line 343 in rpc/backend/comet_to_eth.go needs to be changed, with no need for import or additional definition changes.
-
Copy modified line R343
| @@ -340,7 +340,7 @@ | ||
|
|
||
| // BlockBloom query block bloom filter from block results | ||
| func (b *Backend) BlockBloomFromCometBlock(ctx context.Context, blockRes *cmtrpctypes.ResultBlockResults) (result ethtypes.Bloom, err error) { | ||
| ctx, span := tracer.Start(ctx, "BlockBloomFromCometBlock") | ||
| _, span := tracer.Start(ctx, "BlockBloomFromCometBlock") | ||
| defer func() { evmtrace.EndSpanErr(span, err) }() | ||
|
|
||
| for _, event := range blockRes.FinalizeBlockEvents { |
| func (b *Backend) GetCode(address common.Address, blockNrOrHash rpctypes.BlockNumberOrHash) (hexutil.Bytes, error) { | ||
| blockNum, err := b.BlockNumberFromComet(blockNrOrHash) | ||
| func (b *Backend) GetCode(ctx context.Context, address common.Address, blockNrOrHash rpctypes.BlockNumberOrHash) (bz hexutil.Bytes, err error) { | ||
| ctx, span := tracer.Start(ctx, "GetCode", trace.WithAttributes(attribute.String("address", address.String()), attribute.String("blockNorHash", unwrapBlockNOrHash(blockNrOrHash)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like the End() and EndErr()
rpc/backend/account_info.go
Outdated
|
|
||
| ctx := rpctypes.ContextWithHeight(height) | ||
| clientCtx := b.ClientCtx.WithHeight(height) | ||
| ctx = rpctypes.ContextWithHeight(height, ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this argument ordering is weird
rpc/types/block.go
Outdated
| // 0, it will return an empty context and the gRPC query will use the latest block height for querying. | ||
| // Note that all metadata is processed and removed by the CometBFT layer, so it won't be accessible at gRPC server level. | ||
| func ContextWithHeight(height int64) context.Context { | ||
| func ContextWithHeight(height int64, ctxs ...context.Context) context.Context { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it is ok to break here instead of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func (b *Backend) GetCode(address common.Address, blockNrOrHash rpctypes.BlockNumberOrHash) (hexutil.Bytes, error) { | ||
| blockNum, err := b.BlockNumberFromComet(blockNrOrHash) | ||
| func (b *Backend) GetCode(ctx context.Context, address common.Address, blockNrOrHash rpctypes.BlockNumberOrHash) (bz hexutil.Bytes, err error) { | ||
| ctx, span := tracer.Start(ctx, "GetCode", trace.WithAttributes(attribute.String("address", address.String()), attribute.String("blockNorHash", unwrapBlockNOrHash(blockNrOrHash)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be good to make a trace wrapper in the sdk?
Description
adds otel tracers to the RPC package.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
mainbranch